Fixing error in construction of LegacyFido2ApiManager #2654
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR prevents casting exceptions when creating LegacyFido2ApiManager by adding explicit runtime checks for the activity and fragment types, and updates credential logic to respect the presence of a legacy manager.
- Guard
LegacyFido2ApiManagerconstruction withinstanceofchecks on bothAuthorizationActivityandWebViewAuthorizationFragment - Change
preferImmediatelyAvailableCredentialsto only be true if a legacy manager exists - Record the bug fix in
changelog.txt
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java | Introduce currentActivity var and additional instanceof checks before calling LegacyFido2ApiManager |
| common/src/main/java/com/microsoft/identity/common/internal/fido/CredManFidoManager.kt | Update preferImmediatelyAvailableCredentials to include legacyManager != null |
| changelog.txt | Add a [PATCH] Fixing error in construction of LegacyFido2ApiManager entry |
Comments suppressed due to low confidence (3)
common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java:233
- No unit test currently verifies the fallback path when the fragment is not a WebViewAuthorizationFragment. Adding a test for that branch would ensure we don’t hit a casting exception in other activity flows.
currentActivity instanceof AuthorizationActivity && ((AuthorizationActivity) currentActivity).getFragment() instanceof WebViewAuthorizationFragment
common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java:233
- [nitpick] Consider extracting the fragment into a local variable (e.g.,
Fragment fm = ((AuthorizationActivity) currentActivity).getFragment()) to avoid repeated casts and improve readability.
currentActivity instanceof AuthorizationActivity && ((AuthorizationActivity) currentActivity).getFragment() instanceof WebViewAuthorizationFragment
common/src/main/java/com/microsoft/identity/common/internal/fido/CredManFidoManager.kt:87
- The variable 'legacyManager' is not defined in this scope, which will cause a compilation error. Consider passing 'legacyManager' into the CredManFidoManager constructor or otherwise making its presence available here.
preferImmediatelyAvailableCredentials = (legacyManager != null && getFlightsProvider().isFlightEnabled(CommonFlight.ENABLE_LEGACY_FIDO_SECURITY_KEY_LOGIC) && Build.VERSION.SDK_INT < Build.VERSION_CODES.UPSIDE_DOWN_CAKE)
mohitc1
approved these changes
May 30, 2025
fadidurah
approved these changes
Jun 2, 2025
fadidurah
approved these changes
Jun 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
While testing for their passkey enablement rollout, the Office team found a bug where if they're using OneAuth with fragment or dialog mode (and thus using their own Activity instead of AuthorizationActivity), they run into a casting exception in the process of creating a LegacyFido2ApiManager, as we need to pass a WebViewAuthorizationFragment into the constructor.
The devices which would run into this error would be:
The fix for this is to add explicit type checks before creating the LegacyFido2ApiManager. Because the legacy FIDO2 API does require a Fragment which has a particular object instantiated (which I had added into our WebViewAuthorizationFragment), we can't just pass any fragment from any activity. This means that for Office's case, for Android 13 and below devices, they are going to only use CredMan- which is actually a good thing, since CredMan now has support for security keys for all OS versions (we originally included the legacy API because this wasn't the case a while ago).
At some point, it would be best to just remove the legacy API logic, but that can be done once a larger majority of users are on Android 14+.
Noting again that brokered auth scenarios should remain unaffected because the legacy API path was never hit (the flight has remained off, and default value is false). The same should be the case for Android 14+.